refactor(walk): make the boundary-crossing forms the traversal primitives#35
Merged
Conversation
The three transformers that must cross the subquery scope boundary (CTE hoisting, INTO removal, ILIKE -> LIKE) each hand-listed the five subquery-bearing SqlExpression variants. That compiles clean -- and silently stops crossing -- the day a sixth is added, which is exactly the R3 failure mode: PR #33 fixed two live bugs of that shape. Invert it. map_children_crossing / visit_children_crossing take a second closure for the nested statement and are now the primitives; map_children is that with an identity handler, visit_children with a no-op one. The variant list exists once, in walk.rs, so adding one is a compile error in a single place. Two things the signature is forced into: - Both closures take an explicit ctx rather than capturing. CTEHoister's recursion is &mut self, and two closures each capturing self mutably will not borrow-check; the same applies to &mut deps on the visit path. ctx gives one mutable borrow that the helper splits across the calls. - f_stmt takes and returns the Box, not the statement. By value, the opaque path -- which is most transformers -- would unbox, free and realloc a large struct at every subquery. Passing the Box keeps the identity handler a true passthrough. Behaviour is unchanged; the three transformers already crossed. Tests pin the property the split exists to provide: the variants map_children treats as opaque must be reachable via the crossing forms, and the tuple forms must yield both their same-scope operands and their statement. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
TimelordUK
added a commit
that referenced
this pull request
Jul 18, 2026
docs(refactoring): refresh R2 status after #35
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Next slice of R2, following #33.
The problem
The three transformers that must cross the subquery scope boundary — CTE hoisting, INTO removal,
ILIKE->LIKE— each hand-listed the five subquery-bearingSqlExpressionvariants. That arrangement compiles clean and silently stops crossing the day a sixth is added (Exists, for P3). It's the R3 failure mode precisely: #33 fixed two live bugs of exactly that shape (ILIKEinsideOVER (ORDER BY ...),INTOinside a tupleIN).The change
Invert which form is primitive.
map_children_crossing/visit_children_crossingtake a second closure for the nested statement;map_childrenis now that with an identity handler andvisit_childrenwith a no-op one. The variant list is written down once, inwalk.rs— adding one is a compile error in a single place.hoist_from_expressiongoes from 37 lines to 6; net −224 lines of copy-pasted match arms.Two forced signature details
ctxinstead of capturing.CTEHoister's recursion is&mut self, and two closures each capturingselfmutably won't borrow-check — same for&mut depson the visit path.ctxgives one mutable borrow that the helper splits across the two calls.f_stmttakes and returns theBox, not the statement. By value, the opaque path — most transformers — would unbox, free and realloc a large struct at every subquery. Passing theBoxkeeps the identity handler a genuine passthrough.Both are documented in the module docs and the R2 entry.
Verification
Behaviour is unchanged — all three transformers already crossed the boundary; this only moves where the variants are listed. New tests pin the property the split exists to provide: the variants
map_childrentreats as opaque must be reachable through the crossing forms, and the tuple forms must yield both their same-scope operands and their statement.cargo test: 685 unit tests pass;cargo fmt --checkandclippyclean on the touched filesILIKEinside anINsubquery still matches case-insensitivelyLocal Windows runs show ~21 failures in the Python/examples/history suites; all are environmental (missing
.exesuffix, cp1252 decoding of the CLI's stdout, a#[cfg(unix)]-onlyHOMEredirect) and all are green in CI. Filed separately from this work.🤖 Generated with Claude Code